ποΈGitΠ―ΡΠ°ποΈ
Commit dce2d89c621adb41683ee77d438b865f7ce9fda1
Parents : 8a4e2e3
Author : James Rich <2199651+jamesarich@users.noreply.github.com>
Signature : Signature validation error
Date : 2026-07-28T18:17:25-05:00
Committer : GitHub <noreply@github.com>
Date : 2026-07-28T23:17:25Z
chore(review): cut CodeRabbit round-trips and enforce the repeat defect classes (#6504)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Changes
4 files changed, 145 insertions(+), 10 deletions(-)
Diff
diff --git a/.coderabbit.yaml b/.coderabbit.yaml
index 35d9616c92..1c43c99424 100644
--- a/.coderabbit.yaml
+++ b/.coderabbit.yaml
@@ -13,6 +13,10 @@ reviews:
auto_review:
enabled: true
drafts: false
+ # Review once when the PR goes ready, then on demand via `@coderabbitai review`.
+ # Re-reviewing every push turned 6-commit PRs into 8 review rounds, because each
+ # fix commit reopened a full pass. Batch the fixes, push, then ask for one re-review.
+ auto_incremental_review: false
ignore_usernames:
- renovate
- renovate[bot]
@@ -69,18 +73,20 @@ reviews:
- path: feature/car/
instructions: Run unit tests with `./gradlew :feature:car:testGoogleDebugUnitTest`, and keep Robolectric pinned to SDK 36 for this module.
- # CI is the source of truth for detekt + spotless (Zero Lint Tolerance gate),
- # so disable detekt here to avoid duplicate comments. Keep the security/CI/shell
- # scanners CI doesn't run inline.
+ # Every CodeRabbit tool is enabled by default, so this block only ever needs to
+ # turn things OFF or configure them. detekt is off because CI owns it (Zero Lint
+ # Tolerance gate) and duplicate comments were the noise we removed. The scanners
+ # CI doesn't run β gitleaks, shellcheck, actionlint, zizmor, semgrep, trivy,
+ # presidio (PII), buf (protobuf) β are already on by default; don't re-list them.
tools:
detekt:
enabled: false
- gitleaks:
- enabled: true
- shellcheck:
- enabled: true
- actionlint:
- enabled: true
+ # Custom AST rules mechanically enforce the recurring defect classes that prose
+ # can't. See .coderabbit/ast-grep-rules/ and .skills/code-review/SKILL.md.
+ # essential_rules stays on (default) β these are additive.
+ ast-grep:
+ rule_dirs:
+ - ".coderabbit/ast-grep-rules"
# Auto-generated docstrings/tests/autofix are noisy for a repo with strict
# human-authored KDoc and KMP-aware tests; leave finishing touches off.
@@ -91,12 +97,45 @@ reviews:
enabled: false
# No KDoc-coverage mandate in this repo; the default warning-at-80% check
- # would nag every PR.
+ # would nag every PR. PR titles are already linted by CI
+ # (.github/workflows/pull-request-target.yml), so no title check here either.
pre_merge_checks:
docstrings:
mode: "off"
+ # The two defect classes that survive review-by-prose because they are about
+ # what's ABSENT from a diff β a sibling call site left unfixed, or a test that
+ # would still pass with the fix reverted. Warning, not error: these are
+ # judgment calls and a false positive must not block a merge.
+ custom_checks:
+ - name: "Sibling call sites and presence semantics"
+ mode: "warning"
+ instructions: >-
+ When a diff changes how an absent value is represented β making a field nullable,
+ removing a zero-guard, or adding a presence check β verify EVERY call site of that
+ field was updated, not just the one the bug was reported against. Ambient temperature
+ was fixed in NodeItem.kt while its sibling NodeItemCompact.kt kept the zero-guard.
+ Name any unfixed sibling explicitly. Also flag a new field defaulting to 0 where 0 is
+ a physically reachable value on that scale (RSSI, temperature, current, voltage,
+ particulate concentration). Two exceptions, do NOT flag either: humidity, where 0 %RH
+ is unreachable and the guard is intentional and tested; and the proto `rx_snr`, which
+ has no presence upstream, so its 0f ambiguity cannot be fixed app-side. An app-level
+ SNR field that IS nullable is still in scope.
+ - name: "Tests prove the path, not the end state"
+ mode: "warning"
+ instructions: >-
+ For each added or changed test, decide whether it would still pass if the production
+ code it covers were reverted. Flag tests that seed a fake's backing store and then
+ assert the value comes back, tests that assert only a collection's size rather than
+ which items survived, and tests asserting emission ORDER under Dispatchers.Unconfined
+ (not a stable contract). A test must assert the side effect only the intended path
+ produces β a call counter, a request issued, a cache written.
knowledge_base:
+ # Learnings are how a confirmed finding stops recurring on the next PR. Pin the
+ # scope to this repo: the default `auto` already resolves to `local` for public
+ # repos, but being explicit keeps it from shifting if visibility ever changes.
+ learnings:
+ scope: local
# Feed CodeRabbit the same guidance human/AI contributors follow, including
# the repo-specific .skills/ modules and Copilot path instructions it
# wouldn't pick up by default.
diff --git a/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-float.yml b/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-float.yml
new file mode 100644
index 0000000000..f11268adab
--- /dev/null
+++ b/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-float.yml
@@ -0,0 +1,19 @@
+# Class A of "Recurring Defect Classes" in .skills/code-review/SKILL.md, enforced mechanically.
+# 0 is a real reading on these scales, so a zero-guard silently drops a genuine measurement.
+# Humidity is deliberately excluded: 0 %RH is not physically reachable, and
+# EnvironmentMetricsForGraphingTest.humidity_zeroFilteredOut asserts that guard on purpose.
+id: presence-vs-sentinel-zero-float
+language: kotlin
+severity: warning
+message: >-
+ Zero-guard on a zero-inclusive scale drops real readings. 0 is valid for temperature,
+ current, voltage and soil moisture, so `(x ?: 0f) != 0f` hides a genuine measurement and
+ conflates it with "not reported". Use a null check β `x?.let { }` β as NodeItem.kt does for
+ ambient temperature. Humidity is the one legitimate exception and is excluded from this rule.
+note: >-
+ See .skills/code-review/SKILL.md, "Recurring Defect Classes", class A.
+rule:
+ pattern: "($FIELD ?: 0f) != 0f"
+constraints:
+ FIELD:
+ regex: "\\.(temperature|soil_temperature|co2_temperature|current|voltage|soil_moisture)$"
diff --git a/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml b/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml
new file mode 100644
index 0000000000..758a783ae4
--- /dev/null
+++ b/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml
@@ -0,0 +1,33 @@
+# Class A of "Recurring Defect Classes" in .skills/code-review/SKILL.md, enforced mechanically.
+# Presence in this repo is nullability: the models are Wire-generated, so there is no hasX()
+# accessor β an optional field is simply a nullable type. The proto `rx_rssi` only becomes `Int?`
+# in protobufs 2.7.26.138 (PR #6498, still open at time of writing); on the pinned 2.7.26.130 it
+# is `Int = 0`. So this rule currently guards APP-level nullable rssi (parameters, state, BLE
+# advertisement values) and becomes a proto regression guard once #6498 lands.
+#
+# INT LITERALS ONLY, deliberately. RSSI is an integer dBm value everywhere in this repo β
+# proto `rx_rssi: Int`, `Node.rssi: Int`, `MetricFormatter.rssi(value: Int)` β and there is no
+# Float-typed rssi declaration anywhere. Adding `?: 0f` / `takeIf { it != 0f }` variants here
+# would be dead patterns. Float sentinels on genuinely-Float metrics are the float rule's job.
+# RSSI only, deliberately NOT snr: rx_snr still has no proto presence upstream, so a 0f guard there
+# is genuinely ambiguous and unfixable app-side β flagging it would be pure noise on code nobody can
+# correct. Also excludes `replyId ?: 0` and `packet.from.takeIf { it != 0 }`, which are legitimate
+# because 0 there means "unset", not "a measurement of zero".
+id: presence-vs-sentinel-zero-signal
+language: kotlin
+severity: warning
+message: >-
+ Zero-default on a signal metric conflates "not reported" with a real reading. 0 dBm is a
+ valid RSSI (SX126x reports exactly 0, SX127x can go positive) and on a signal-strength scale
+ it renders as the STRONGEST value β so an unknown signal displays as excellent. Keep the type
+ nullable and branch on null, rendering absence explicitly rather than substituting a number.
+note: >-
+ See .skills/code-review/SKILL.md, "Recurring Defect Classes", class A. rx_snr still has no
+ proto presence upstream, so its 0f ambiguity cannot be fixed app-side β do not re-raise that.
+rule:
+ any:
+ - pattern: "$RECV.takeIf { it != 0 }"
+ - pattern: "$RECV ?: 0"
+constraints:
+ RECV:
+ regex: "(?i)rssi$"
diff --git a/.skills/code-review/SKILL.md b/.skills/code-review/SKILL.md
index e58934add9..0be540ed14 100644
--- a/.skills/code-review/SKILL.md
+++ b/.skills/code-review/SKILL.md
@@ -3,6 +3,50 @@
## Description
Perform comprehensive code reviews for `Meshtastic-Android`, ensuring changes adhere to KMP architecture, Kotlin Multiplatform conventions, MAD standards, and CMP best practices.
+## Recurring Defect Classes (check these first)
+
+These four classes account for most of the Major findings raised on recent PRs, and they recur because the compiler, `detekt`, and `spotless` cannot see any of them. Check them while *writing* the code, not only while reviewing it.
+
+### A. Presence vs. sentinel zero
+**A numeric field whose absence matters must be nullable. Never let `0` stand in for "not reported".**
+
+`0` is a legitimate reading for RSSI (0 dBm), SNR, temperature, and air-quality concentration, so a `0` default silently merges "no data" with a real measurement. Both directions are bugs: an absent value gets persisted and displayed as a real one, and a genuine `0` gets discarded by a `takeIf { it != 0 }` guard.
+
+- [ ] **Accumulators and defaults:** a field collected over time defaults to `null`, not `0`. Absence checks read `== null` and presence checks `!= null` β never `== 0` for either.
+- [ ] **Aggregates over empty sets:** median/mean/min helpers return `T?` and propagate `null` for an empty input. A `0` fallback biases the result in whichever direction the comparator sorts β under the higher-is-better RSSI ordering used for ranking, an empty set's `0` outranks a real `-80 dBm`; under a plain `min` it would instead win as the smallest. Either way the missing value competes as if measured.
+- [ ] **Comparators:** sort missing values explicitly last; do not let them fall through to a numeric default.
+- [ ] **No zero-guards on real scales:** `takeIf { it != 0 }` is only valid where `0` is genuinely impossible. On any signed or zero-inclusive scale it destroys data.
+- [ ] **Proto presence:** when a proto field gains explicit presence, adopt the nullable accessor everywhere rather than keeping a `0` comparison. Fields with *no* presence cannot be fixed app-side β say so rather than faking it.
+- [ ] **Tests:** a nullable numeric needs **both** a null case and a zero-value case. One without the other does not pin the distinction.
+
+### B. Readβdecideβwrite across a suspend boundary
+**Read the current value, decide, and write inside a single `dataStore.edit { }` block.**
+
+Reading a `StateFlow` (or a prior `suspend` getter), branching on it, and then issuing separate writes leaves a window where a concurrent change interleaves β so a guard can fire against state that no longer exists and clobber a user preference. `NotificationPrefsImpl.setGeofenceAlertOptIn` (`core/prefs/β¦/notification/NotificationPrefsImpl.kt`) is the reference example: it parses, mutates, caps, and writes in one `edit`.
+
+- [ ] Any "if the flag is X, set Y and Z" transition happens inside one `edit`/`updateData` lambda.
+- [ ] The decision reads the block's own `prefs`/`current` snapshot, **not** a cached `StateFlow` value from outside.
+- [ ] Multi-key transitions are one `edit` call, not several `scope.launch` writes.
+- [ ] Radio-side config mutations go through a single `editSettings { }` transaction (see `AdminController.editSettings`).
+
+### C. Tests that pass for the wrong reason
+**Assert that the intended code path produced the result β not merely that the result exists.**
+
+Seeding a fake's backing store and then asserting the value comes back passes even if the production code under test is deleted. The test must fail when the path breaks.
+
+- [ ] **Prove the path ran:** assert the side effect that only the intended path produces (a call counter incremented, a request issued, a cache written) alongside the observed value.
+- [ ] **Don't pre-seed the answer:** drive the value in through the path being tested (a gated fake response) instead of injecting it directly into the cache.
+- [ ] **Isolate the variable:** a test for one field must not let a second differing field explain the assertion.
+- [ ] **Assert survivors, not just counts:** for dedup/merge logic, assert the identities that remain, not the size.
+- [ ] **No ordering assertions under `Dispatchers.Unconfined`:** emission order is not a stable contract there β assert final state.
+
+### D. Room schema bump without a migration test
+**Every schema-version increment ships a migration test that proves existing rows survive.**
+
+- [ ] A new `core/database/schemas/<n>.json` is accompanied by an `(n-1)βn` test in `core/database/src/androidHostTest/.../*MigrationTest.kt`.
+- [ ] The test inserts rows at the old version, migrates, and asserts **row count and column values** are preserved β not merely that the migration runs.
+- [ ] Columns going nullable assert that pre-existing values are retained and that the new `NULL` state is reachable (this is class A at the storage layer).
+
## Code Review Checklist
When reviewing code, meticulously verify the following categories. Flag any deviations and propose the canonical project pattern as a fix.
Served by rngit 1.5.4 - Generated in 0.06s